Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: PyPy support #527

Merged
merged 14 commits into from
Dec 16, 2016
Merged

WIP: PyPy support #527

merged 14 commits into from
Dec 16, 2016

Conversation

wjakob
Copy link
Member

@wjakob wjakob commented Nov 24, 2016

This commit includes a few minor modifications to pybind11 that are needed to get simple hello-world style functions to compile and run on the latest PyPy. Types are now supported as well.

The test suite compiles but crashes when executed.

@wjakob
Copy link
Member Author

wjakob commented Dec 3, 2016

A progress update here:

  1. PyPy now passes 95% of the test suite 🎉 .

  2. A few somewhat intrusive changes were needed to accomplish this. In the (non-PyPy) pybind11 code, when the user calls a function like def_property_static() or def_buffer(), a custom metaclass or buffer API is "snuck" into the type after it has already been created via PyType_Ready by overwriting pointers in the type data structure.

    In PyPy, we can't get away with that sort of thing anymore. The elementary type properties must be known when it is created. Thus, I've introduced two new class_<> attributes: buffer_protocol and metaclass that must be specified if the buffer protocol or static properties are used, respectively. Omitting these will lead to an (informative) exception being thrown at module import time.

    This will affect everyone, so any feedback here is welcomed.

  3. NumPy integration also works. There is one memory leak in test_to_python() that is potentially a PyPy bug, I plan to file a bug. Also, test_cast_numpy_int64_to_uint64() fails for reasons unknown to me.

  4. PyPy really doesn't seem to like C++ types with multiple inheritance exposed via cpyext. All of the multiple inheritance-related testcases are skipped for now.

  5. PyPy doesn't seem to register class docstrings for some reason.

  6. The garbage collector is a lot more finicky. I've needed to add extra gc.collect() calls in many places in the test suite to prevent the memory leak detection from detecting false positives.

  7. I've inadvertently introduced some regressions, so this currently breaks some things on regular Python ;)

Cheers,
Wenzel

@dean0x7d
Copy link
Member

dean0x7d commented Dec 3, 2016

Nice! That went pretty quickly from "Hello, World" to 95% of the test suite :)

Just a quick comment regarding number 6: gc.collect() returns an integer indicating the number of unreachable objects found in that pass. So the following should ensure a full cleanup:

while gc.collect():
    pass

@wjakob
Copy link
Member Author

wjakob commented Dec 3, 2016

Just a quick comment regarding number 6: gc.collect() returns an integer indicating the number of unreachable objects found in that pass. So the following should ensure a full cleanup:

while gc.collect():
    pass

It doesn't work. PyPy always returns zero from gc.collect()

@wjakob
Copy link
Member Author

wjakob commented Dec 3, 2016

I filed a bug report regarding the reference counting leak involving the buffer protocol: https://bitbucket.org/pypy/pypy/issues/2444/pypy-cpyext-reference-counting-leak-when

@wjakob
Copy link
Member Author

wjakob commented Dec 4, 2016

PyPy bugreport regarding multiple inheritance: https://bitbucket.org/pypy/pypy/issues/2445/support-pybind11-in-conjunction-with-pypys

@mattip
Copy link
Contributor

mattip commented Dec 5, 2016

Hi. PyPY dev here. I am slowly looking into the issues raised. About "PyPy doesn't seem to register class docstrings for some reason" - once PyType_Ready is called, no changes to the c PyTypeObject struct will be reflected into app-level python. About reference counting, we do not support sys.getrefcount() at all and do not promise to collect objects that have been freed. We will not support using del as a mechanism to do anything but free resources.

And in more detail:

We create an internal app-level object when PyType_Ready is called that reflects the c-level object, we track the refcount of the c-level object and when it goes to 0 may delete both objects at our convenience but our garbage collection is a mark-and-sweep scheme, however if there is no memory pressure we may choose to not collect anything. When we do call the del method on objects, it may be during the execution of exit() from the interpreter.

We do not currently have a mechanism to update the app-level object from the c-level object (except in some special cases) after PyType_Ready is called.

@wjakob
Copy link
Member Author

wjakob commented Dec 5, 2016

Hi @mattip,

cool, that's fantastic -- thank you for taking a look. Meanwhile, I've posted a followup to the multiple inheritance issue on the PyPy issue tracker.

I've created a new ticket about the issue involving tp_doc. I double-checked, and we're filling out all of the fields before the PyType_Ready call, hence it's strange that the docstrings don't show up in Python: https://bitbucket.org/pypy/pypy/issues/2446/cpyext-tp_doc-field-not-reflected-on

The design decisions regarding the cpyext and type construction make sense, and part of the changes in this PR are to make pybind11 deal with that (i.e. not rely on patching up type objects after creating them, which was always kind of hacky in any case)

Thanks,
Wenzel

@wjakob wjakob force-pushed the master branch 3 times, most recently from ad797b7 to f3519d6 Compare December 5, 2016 23:23
@dean0x7d
Copy link
Member

dean0x7d commented Dec 6, 2016

Seeing as PyPy doesn't guarantee cleanup until exit and in general GC behavior can vary based on config and platform, it may be good to disable ConstructorStats checks on PyPy. The destructor counts will likely be unreliable going from machine to machine -- no deterministic way to call gc.collect() the correct number of times. CPython's reference counting mostly plays well with C++'s deterministic destruction, but PyPy has a real GC which doesn't really make any cleanup promises.

Perhaps a global option can be added to pytest and guard all ConstructorStats checks (or replace ConstructorStats with a dummy class with a == operator which always returns true).

@wjakob
Copy link
Member Author

wjakob commented Dec 6, 2016

My impression was that PyPy uses an asynchronous tri-color collector that might not have captured an object that has become garbage in the current iteration (the object might have been referenced at the beginning of that GC cycle), but it will do so in the next one at the latest. Having spent quite a while with this, my practical experience was that two calls of gc.collect() are indeed consistently enough, with the failing testcases turning out to be actual leaks. So I think there is some value in having these, at least until these assumptions are actually violated.

@wjakob
Copy link
Member Author

wjakob commented Dec 7, 2016

The two main issues (multiple inheritance, docstrings) are now fixed in PyPy 🎉

That just leaves the refcounting issue involving the buffer protocol, and I think we're ready to ship this: https://bitbucket.org/pypy/pypy/issues/2444/pypy-cpyext-reference-counting-leak-when

@dean0x7d
Copy link
Member

Slightly off-topic, but I noticed that conftest.py was reflowed to a lower line length. I'm just wondering if this is something that should be added to the flake8 configuration in order to more easily maintain the code style (flake8 is currently set to unlimited line length).

@aldanor
Copy link
Member

aldanor commented Dec 12, 2016

Slightly off-topic, but I noticed that conftest.py was reflowed to a lower line length. I'm just wondering if this is something that should be added to the flake8 configuration in order to more easily maintain the code style (flake8 is currently set to unlimited line length).

Agreed. I vouch for linelength of 99 or 100 :)

@wjakob
Copy link
Member Author

wjakob commented Dec 12, 2016

Agreed. I vouch for linelength of 99 or 100 :)

@aldanor: this sounds fine. I don't remember why I reflowed that file, probably it did not fit into my arbitrarily sized editor window :)

@wjakob
Copy link
Member Author

wjakob commented Dec 13, 2016

^ @aldanor, @dean0x7d, @jagerman, @lyskov, @SylvainCorlay, @JohanMabille, @pschella

I believe that the few remaining PyPy integration issues will be addressed shortly (the PyPy devs have been super-responsive and already fixed most of the items I reported)

This means PyPy support can be part of pybind11 2.0 🎉 . Since it's a somewhat intrusive breaking change (new system and public interface for creating objects with metaclasses and supporting the buffer object protocol), it would be good to have some extra pairs of eyes looking at this PR.

Thanks,
Wenzel

@@ -1,10 +1,16 @@
import gc


def collect():
gc.collect()
gc.collect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function also appears in test_keep_alive.py and test_numpy_array.py. To avoid duplication, it's probably worth adding it to pytest_namespace in conftest.py. Then it can be called like pytest.gc_collect().

@@ -111,6 +111,11 @@ def test_property_return_value_policies(access):
assert getattr(obj, access + "_func").value == 1


@pytest.mark.parametrize("access", ["static_ro", "static_rw"])
def test_property_return_value_policies_static(access):
test_property_return_value_policies(access)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can be merged back with the test above now that PyPy doesn't need to skip the static part.

"""When returning an rvalue, the return value policy is automatically changed from
`reference(_internal)` to `move`. The following would not work otherwise.
"""
from pybind11_tests import TestPropRVP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment.

@@ -19,6 +19,7 @@ def test_roundtrip():


def test_roundtrip_with_dict():
return False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this test was being skipped but was reported as 'passed' because there were no assertion errors. Should be checked on PyPy.

@@ -119,8 +119,12 @@ def __repr__(self):
assert cstats.alive() == 0


def test_docs(doc):
# PyPy does not seem to propagate the tp_docs field at the moment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this fixed? Leftover comment?

@lyskov
Copy link
Contributor

lyskov commented Dec 15, 2016

I have tried running my project tests while using this PR code but i am running into what looks like memory error in almost all of them:

*** Error in `/usr/bin/python3': free(): invalid pointer: 0x000000000a5b6668 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7d1fd)[0x7fe440bb21fd]
/home/benchmark/rosetta/binder/main/source/build/PyRosetta/linux/clang/python-3.5/minsizerel/build/rosetta.so(+0x15ccef6)[0x7fe42842cef6]
/home/benchmark/rosetta/binder/main/source/build/PyRosetta/linux/clang/python-3.5/minsizerel/build/rosetta.so(+0x53d766d)[0x7fe42c23766d]
/lib64/libpython3.5m.so.1.0(PyEval_EvalFrameEx+0x655a)[0x7fe4419416ba]
/lib64/libpython3.5m.so.1.0(+0x12984c)[0x7fe44194484c]
/lib64/libpython3.5m.so.1.0(PyEval_EvalCodeEx+0x48)[0x7fe441944958]
/lib64/libpython3.5m.so.1.0(PyEval_EvalCode+0x3b)[0x7fe44194499b]
/lib64/libpython3.5m.so.1.0(+0x148d04)[0x7fe441963d04]
/lib64/libpython3.5m.so.1.0(PyRun_FileExFlags+0x9d)[0x7fe44196617d]
/lib64/libpython3.5m.so.1.0(PyRun_SimpleFileExFlags+0xf7)[0x7fe4419662e7]
/lib64/libpython3.5m.so.1.0(Py_Main+0xe94)[0x7fe44197c1f4]
/usr/bin/python3(main+0x169)[0x400ad9]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fe440b56af5]
/usr/bin/python3[0x400b89]

I will see if i can obtain a better backtrace...

@lyskov
Copy link
Contributor

lyskov commented Dec 15, 2016

ok on Mac OS, i can see extra message: python(89212,0x7fff79c16300) malloc: *** error for object 0x1256963f8: pointer being freed was not allocated

And i got the following backtrace:

Catchpoint 2 (exception thrown).
Catchpoint 2 (exception caught), throw location unknown, catch location unknown, exception type std::__1::bad_weak_ptr
0x00007fff8e12cac2 in __cxa_throw ()
(gdb) bt
#0  0x00007fff8e12cac2 in __cxa_throw ()
#1  0x000000010fb101be in dyld_stub_memchr ()
#2  0x000000010d837d7f in dyld_stub_memchr ()
#3  0x000000010000e50a in PyObject_Call ()
#4  0x00000001000192f7 in PyMethod_New ()
#5  0x000000010000e50a in PyObject_Call ()
#6  0x00000001000557b0 in _PyObject_SlotCompare ()
#7  0x0000000100050f87 in _PyType_Lookup ()
#8  0x000000010000e50a in PyObject_Call ()
#9  0x000000010008b399 in PyEval_EvalFrameEx ()
#10 0x0000000100088352 in PyEval_EvalCodeEx ()
#11 0x0000000100087dcb in PyEval_EvalCode ()
#12 0x000000010009cee3 in PyImport_ExecCodeModuleEx ()
#13 0x000000010009fbc2 in PyImport_AppendInittab ()
#14 0x00000001000a0663 in PyImport_AppendInittab ()
#15 0x00000001000a022d in PyImport_AppendInittab ()
#16 0x000000010009e46e in PyImport_ImportModuleLevel ()
#17 0x00000001000839b3 in _PyBuiltin_Init ()
#18 0x000000010000e50a in PyObject_Call ()
#19 0x000000010008e3df in PyEval_CallObjectWithKeywords ()
#20 0x000000010008a641 in PyEval_EvalFrameEx ()
#21 0x0000000100088352 in PyEval_EvalCodeEx ()
#22 0x0000000100087dcb in PyEval_EvalCode ()
#23 0x00000001000a800e in PyParser_ASTFromFile ()
#24 0x00000001000a7e2a in PyRun_InteractiveOneFlags ()
#25 0x00000001000a7939 in PyRun_InteractiveLoopFlags ()
#26 0x00000001000a77e3 in PyRun_AnyFileExFlags ()
#27 0x00000001000b9437 in Py_Main ()
#28 0x00007fff969ac5c9 in start ()

Please let me know if there is anything else i can do to help

@lyskov
Copy link
Contributor

lyskov commented Dec 15, 2016

The Python line that lead to this is nothing special: pose = Pose() where Pose class is bound like this:

pybind11::class_<core::pose::Pose, std::shared_ptr<core::pose::Pose>> cl(M("core::pose"), "Pose", "...");
pybind11::handle cl_type = cl;
cl.def(pybind11::init<>());
cl.def(pybind11::init<const class core::pose::Pose &>(), pybind11::arg("src"));
cl.def(pybind11::init<const class core::pose::Pose &, unsigned long, unsigned long>(), pybind11::arg("src"), pybind11::arg("residue_begin"), pybind11::arg("residue_end"));
...

@wjakob
Copy link
Member Author

wjakob commented Dec 16, 2016

Right, of course..

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

looks like there is a few new commits now available in wjakob:master. After updating i indeed to longer can see any errors in both Linux and Mac.

@jagerman could it be that you missing that example is using pybind11::return_value_policy::reference_internal?

@jagerman
Copy link
Member

@lyskov - no, the reference_internal isn't the problem, that just uses a keep-alive to delay to Container destruction until after the Data destruction. That part is working: the second Data destruction is happening when Container gets destroyed.

@jagerman
Copy link
Member

After updating i indeed to longer can see any errors in both Linux and Mac.

That's probably just luck, the core problem is still there.

@dean0x7d
Copy link
Member

@jagerman That issue was fixed in #533. Non-owning references don't create holders so there is no double free.

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

hmmm, ok... then i have questions:

  1. why it is working now?
  2. what is the right return-policy for this case?

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

Also, i was under the impression that object held with reference_internal will never be deleted, - is this no longer true?

@jagerman
Copy link
Member

Ah, right, I forgot about that. So yes, it is fixed (but you'd run into it again if you returned a pointer).

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

So, how do i need to bind function that will return pointer to object that should not be deleted?

@jagerman
Copy link
Member

Scratch what I said, you should be fine with a pointer, too.

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

ok, i am re-compiling my tests with an updated version of this PR and will post when i have testing results.

@dean0x7d
Copy link
Member

For never deleting an object, there are two options:

  1. A py::nodelete holder should work with any return value policy.

  2. reference or reference_internal with any other holder type.

As for double delete problems, I believe those are now only a risk with the take_ownership policy.

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

Great, thank you for clarifying this @dean0x7d ! And i will see if i can refactor my code to use py::nodelete instead of T*.

@jagerman
Copy link
Member

Your T* holder declaration isn't doing anything anyway.

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

@jagerman i guess it does not now, but it was indeed needed before. A while back binding like: pybind11::class_<B, B* > cl(m, "B_P"); was not compiling without it.

@dean0x7d
Copy link
Member

Hmm, I think that right now there is actually no difference between:

py::class_<MyClass, std::unique_ptr<MyClass, py::nodelete>>(m, "MyClass")

and

py::class_<MyClass, MyClass*>(m, "MyClass")

Unless I'm overlooking something?

@jagerman
Copy link
Member

I assume you mean with the T* holder declaration (without it you get a static assertion failure).

@dean0x7d
Copy link
Member

Ah, I forgot about the static_assert there. So the holder declaration is not 100% useless, but probably should be avoided.

@lyskov
Copy link
Contributor

lyskov commented Dec 16, 2016

allright, i just run my set of tests with a new version of source and this time i see no issues so as far as i can tell everything is working!

@wjakob wjakob merged commit 1d1f81b into pybind:master Dec 16, 2016
@wjakob
Copy link
Member Author

wjakob commented Dec 16, 2016

This is merged now.

@SylvainCorlay
Copy link
Member

Kudos on this one. This is great that it got through.

@dean0x7d
Copy link
Member

The readme/docs say:

PyPy2.7 >= 5.5

But I think this should be >= 5.7 (future version, current nightly). Last time I checked 5.6 doesn't even compile.

@jagerman
Copy link
Member

I wonder if it would be useful to put a static_assert testing for known-unsupported compilers. E.g. it could catch gcc < 4.8, MSVC < 2015 update 3, ICC < 15. And perhaps protected with a PYBIND11_ENABLE_UNSUPPORTED macro that people could test if they are sure they really want to bypass the checks (with the understanding that they are on their own if they do).

@wjakob
Copy link
Member Author

wjakob commented Dec 18, 2016

@jagerman: sounds like a nice idea!

@wjakob
Copy link
Member Author

wjakob commented Dec 18, 2016

@dean0x7d: fixed, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants